Skip to content

fix(l7): reject requests with both CL and TE headers in inference parser (CWE-444)#671

Open
latenighthackathon wants to merge 1 commit intoNVIDIA:mainfrom
latenighthackathon:fix/inference-cl-te-rejection
Open

fix(l7): reject requests with both CL and TE headers in inference parser (CWE-444)#671
latenighthackathon wants to merge 1 commit intoNVIDIA:mainfrom
latenighthackathon:fix/inference-cl-te-rejection

Conversation

@latenighthackathon
Copy link
Copy Markdown
Contributor

@latenighthackathon latenighthackathon commented Mar 30, 2026 •
edited
Loading

Summary

The CL/TE desynchronisation guard added in #663 for the REST path was not applied to the inference request parser. A request containing both Content-Length and Transfer-Encoding headers is silently accepted by inference.rs, with Content-Length ignored in favour of chunked transfer-encoding. This enables HTTP request smuggling if an upstream server interprets the request via Content-Length instead (CWE-444, RFC 7230 Section 3.3.3).

Related Issue

Closes #670

Follow-up to #663 / #637 — same vulnerability class, sister parser.

Changes

  • Added CL+TE dual-header rejection check in try_parse_http_request() after header parsing loop, returning ParseResult::Invalid when both is_chunked and has_content_length are true.
  • Added reject_dual_content_length_and_transfer_encoding and reject_dual_transfer_encoding_and_content_length tests mirroring SEC-009 coverage in rest.rs.
  • Added te_substring_not_chunked test verifying partial TE tokens like chunkedx are not treated as chunked.
  • Aligned error message casing with rest.rs convention ("Request contains..." instead of "request contains...").

Testing

  • mise run pre-commit passes (format, license, lint checks)
  • Unit tests added/updated

Executed:

  • mise run pre-commit locally: rust:format:check, license:check, python:format:check, helm:lint all pass
  • Compile/test steps require Linux CI runners
  • Verified all three new tests exercise the intended code paths
  • Confirmed existing tests (duplicate CL, non-numeric CL, chunked parsing) are unaffected

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@latenighthackathon latenighthackathon requested a review from a team as a code owner March 30, 2026 05:03
@latenighthackathon
Copy link
Copy Markdown
Contributor Author

latenighthackathon commented Mar 30, 2026

Self-review follow-up in b2b3d61:

  1. Capitalised error message — "Request contains both..." now matches the casing convention in rest.rs (line 267) for the equivalent guard.
  2. Added te_substring_not_chunked test — verifies that a partial Transfer-Encoding value like chunkedx is not mistakenly treated as chunked. Mirrors the same test in rest.rs (line 758). The inference parser already handles this correctly via .eq_ignore_ascii_case("chunked") on split values, but there was no test coverage.

@johntmyers johntmyers self-assigned this Mar 30, 2026
johntmyers
johntmyers reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@johntmyers johntmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix — the guard logic is correct and well-placed, and the tests cover the right cases (header order independence + TE substring rejection).

Two minor items:

  1. Whitespace in te_substring_not_chunked test — The format! macro's line continuation introduces leading spaces before {body}, so the parsed body ends up as spaces + truncated JSON rather than the intended payload. The test still passes (Content-Length controls read size) but is misleading. Quick fix:

    let request = format!(
    "POST /v1/chat/completions HTTP/1.1\r\n\
     Host: x\r\n\
     Transfer-Encoding: chunkedx\r\n\
     Content-Length: {}\r\n\
     \r\n{body}",
    body.len(),
);

  2. Consider squashing the two commits (guard + capitalization fix) into one before merge.

Also — please run mise run pre-commit before pushing, per AGENTS.md. We need to confirm lint/format/license checks pass locally before we approve workflows to run on this PR.

@latenighthackathon
fix(l7): reject requests with both CL and TE headers in inference par…
f794e25 
…ser (CWE-444)

The CL/TE desynchronisation guard added in NVIDIA#663 for the REST path was
not applied to the inference request parser.  A request containing both
Content-Length and Transfer-Encoding headers could be interpreted
differently by the proxy and the upstream server, enabling HTTP request
smuggling (CWE-444, RFC 7230 Section 3.3.3).

Add the same rejection check and tests mirroring the REST parser
coverage, including TE substring validation.

Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
@latenighthackathon latenighthackathon force-pushed the fix/inference-cl-te-rejection branch from 52739de to f794e25 Compare March 30, 2026 15:09
@latenighthackathon
Copy link
Copy Markdown
Contributor Author

latenighthackathon commented Mar 30, 2026 •
edited
Loading

@johntmyers Thanks for the review! Both items addressed in the squashed commit:

  1. Whitespace fix — removed the leading spaces before {body} by placing \r\n on its own continuation line so the body starts immediately after the blank line.
  2. Squashed all three commits into one.

Ran mise run pre-commit locally — rust:format:check, license:check, python:format:check, and helm:lint all pass. The compile/link/test steps fail due to Windows link.exe conflict (Linux-only project), but CI covers those.

@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johntmyers johntmyers johntmyers left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@johntmyers johntmyers

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(l7): inference parser missing CL+TE dual-header rejection (CWE-444)

2 participants

@latenighthackathon @johntmyers